Skip to content

Bug: Save original index and remap after function completes #61116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Apr 15, 2025
Merged

Bug: Save original index and remap after function completes #61116

merged 7 commits into from
Apr 15, 2025

Conversation

Jeffrharr
Copy link
Contributor

@Jeffrharr Jeffrharr commented Mar 13, 2025 •
edited
Loading

Note: I'm new to this project, so this is my first PR.

Saves the index for SeriesNLargest at algorithm start and resets it before returning. This fixes performance issues when the index has many duplicate values.

Results:

  • The original statistics can be viewed in the original ticket, but slow_df was several ms. Note that the results in the ticket were not from my development machine and specific timings differ.
In [4]: import pandas as pd
   ...: import numpy as np
   ...: 
   ...: N = 1500
   ...: N_HALF = 750
   ...: 
   ...: slow_df = pd.DataFrame({'a':  np.random.rand(N)}, index=np.concatenate([[1] * N_HALF, np.arange(N_HALF)]))
   ...: print("slow_df")
   ...: %timeit slow_df['a'].nlargest()
   ...: 
   ...: fast_df = pd.DataFrame({'a': np.random.rand(N)})
   ...: print("fast_df")
   ...: %timeit fast_df['a'].nlargest()

slow_df
427 μs ± 11.4 μs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)
fast_df
420 μs ± 5.4 μs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

Tests

The existing tests should cover this unless we want to add specific tests via the asv_bench.

Addendum

I also modified the call to sort to use sort(kind="stable") to get consistent ordering which is what is currently happening in the equivalent Frame method (it was using kind=mergesort which is equivalent to kind=stable, but kept for portability). I can remove this -- it may be better in another PR.
https://numpy.org/doc/stable/reference/generated/numpy.sort.html#numpy.sort

@Jeffrharr Jeffrharr marked this pull request as ready for review March 13, 2025 22:13
@Jeffrharr Jeffrharr changed the title Save original index and remap after function completes. Bug: Save original index and remap after function completes Mar 14, 2025
@rhshadrach rhshadrach added Performance Memory or execution speed performance Filters e.g. head, tail, nth labels Mar 26, 2025
rhshadrach
rhshadrach requested changes Mar 26, 2025
View reviewed changes
Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I'm seeing a performance regression when the index does not contain duplicates. Can we do this conditionally.

@Jeffrharr
Copy link
Contributor Author

Jeffrharr commented Mar 26, 2025 •
edited
Loading

Thanks for the PR! I'm seeing a performance regression when the index does not contain duplicates. Can we do this conditionally.

I appreciate the review!

Can you confirm the performance regression numbers that you are seeing? Note that the timings in the original ticket were from a different machine than the one I'm working on.

ASV Benchmarks appear similar enough to be reasonable in the standard case.

-- Original Function --

[37.66%] ··· series_methods.NSort.time_nlargest                              ok
[37.66%] ··· ======= ==========
               keep            
             ------- ----------
              first   3.59±0ms 
               last   3.70±0ms 
               all    3.84±0ms 
             ======= ==========

[37.71%] ··· series_methods.NSort.time_nsmallest                             ok
[37.71%] ··· ======= ==========
               keep            
             ------- ----------
              first   3.26±0ms 
               last   2.94±0ms 
               all    3.31±0ms 
             ======= ==========

-- Function with my changes --

[37.66%] ··· series_methods.NSort.time_nlargest                              ok
[37.66%] ··· ======= ==========
               keep            
             ------- ----------
              first   3.71±0ms 
               last   3.41±0ms 
               all    3.60±0ms 
             ======= ==========

[37.71%] ··· series_methods.NSort.time_nsmallest                             ok
[37.71%] ··· ======= ==========
               keep            
             ------- ----------
              first   2.86±0ms 
               last   2.58±0ms 
               all    3.20±0ms 
             ======= ==========

@Jeffrharr Jeffrharr requested a review from rhshadrach March 26, 2025 21:56
@Jeffrharr
Copy link
Contributor Author

Thanks for the PR! I'm seeing a performance regression when the index does not contain duplicates. Can we do this conditionally.

Hey, can I get a re-review on this?

@rhshadrach
Copy link
Member

Thanks for the ping, will get to it in the next day or two.

rhshadrach
rhshadrach approved these changes Apr 12, 2025
View reviewed changes
Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@rhshadrach rhshadrach modified the milestones: 2.3, 3.0 Apr 12, 2025
mroeschke
mroeschke reviewed Apr 14, 2025
View reviewed changes
doc/source/whatsnew/v3.0.0.rst
@@ -61,6 +61,7 @@ Other enhancements
- :meth:`Series.cummin` and :meth:`Series.cummax` now supports :class:`CategoricalDtype` (:issue:`52335`)
- :meth:`Series.plot` now correctly handle the ``ylabel`` parameter for pie charts, allowing for explicit control over the y-axis label (:issue:`58239`)
- :meth:`DataFrame.plot.scatter` argument ``c`` now accepts a column of strings, where rows with the same string are colored identically (:issue:`16827` and :issue:`16485`)
- :meth:`Series.nlargest` uses a 'stable' sort internally and will preserve original ordering.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- :meth:`Series.nlargest` uses a 'stable' sort internally and will preserve original ordering.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mroeschke - there is a change in Series.nlargest here where previously we weren't using a stable sorting algorithm in certain cases. Now the only dtype I think this can possibly effect is object (as otherwise equal implies identical), and even there it is quite uncommon to have an impact. Perhaps the line needs to make this more clear that it is unlikely to have any impact, or are you thinking it isn't even worth mentioning?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah gotcha. OK doesnt hurt to include it in the whatsnew notes

@Jeffrharr Jeffrharr force-pushed the selectn_series_perf_impact branch from 7e9a2a3 to 8aa2a2c Compare April 14, 2025 22:56
@mroeschke mroeschke merged commit d739c92 into pandas-dev:main Apr 15, 2025
42 checks passed
@mroeschke
Copy link
Member

Thanks @Jeffrharr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhshadrach rhshadrach rhshadrach approved these changes

@mroeschke mroeschke mroeschke approved these changes

Assignees
No one assigned
Labels
Filters e.g. head, tail, nth Performance Memory or execution speed performance
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

PERF: Surprisingly slow nlargest with duplicates in the index
3 participants
@Jeffrharr @rhshadrach @mroeschke